Skip to content

refactor(tests): replace mailer mocks with email delivery assertions#2617

Merged
olleolleolle merged 11 commits into
codebar:masterfrom
mroderick:refactor/mailer-tests
May 27, 2026
Merged

refactor(tests): replace mailer mocks with email delivery assertions#2617
olleolleolle merged 11 commits into
codebar:masterfrom
mroderick:refactor/mailer-tests

Conversation

@mroderick
Copy link
Copy Markdown
Collaborator

@mroderick mroderick commented May 14, 2026

Summary

Replaces mock-based mailer tests with assertions on ActionMailer::Base.deliveries. This improves test isolation and eliminates flaky tests in parallel test execution.

Problem

The existing tests used expect(Mailer).to receive(:method) and expect_any_instance_of(Mailer) which:

  • Cause race conditions in parallel test runs (same mock catches calls from other processes)
  • Test implementation details rather than behavior
  • Are globally scoped, causing interference between tests

Solution

Replace mock expectations with assertions on actual email delivery:

  • Verify email count: expect { action }.to change { ActionMailer::Base.deliveries.count }
  • Verify email recipients: expect(email.to).to include(member.email)
  • Verify email content: expect(email.body.encoded).to match(/pattern/)

Files Changed

File Changes
spec/support/shared_examples/behaves_like_sending_workshop_emails.rb Replace mailer mocks with delivery count assertions
spec/models/invitation_manager_spec.rb Convert all mailer mocks (waiting list, reminders, meetings, async)
spec/models/feedback_request_spec.rb Simplify to single email delivery test
spec/models/eligibility_inquiry_spec.rb Replace spy with delivery assertion
spec/models/attendance_warning_spec.rb Replace spy with delivery assertion
spec/features/subscribing_to_emails_spec.rb Replace all expect_any_instance_of mocks
spec/mailers/member_mailer_spec.rb Replace expect_any_instance_of with content verification

Test Results

All 59 affected tests pass:

  • 37 invitation_manager tests
  • 10 invitation_manager_logging tests
  • 7 feedback_request tests
  • 2 eligibility_inquiry tests
  • 3 attendance_warning tests
  • 8 feature tests (subscriptions)
  • 15 mailer tests

Benefits

  1. Better parallel test isolation - No shared global state from mocks
  2. Tests behavior, not implementation - Verifies emails are actually sent
  3. More resilient - Won't break if internal method names change
  4. Clearer intent - Tests describe what should happen, not how

@mroderick mroderick requested a review from olleolleolle May 14, 2026 13:13
@mroderick
Copy link
Copy Markdown
Collaborator Author

@olleolleolle is this the most Rails idiomatic way of testing email sending?

@mroderick mroderick force-pushed the refactor/mailer-tests branch 2 times, most recently from 476cad2 to 58d14c1 Compare May 26, 2026 19:46
mroderick added 11 commits May 26, 2026 21:49
…d of mocks

Replaces mock expectations on mailer classes with assertions about
ActionMailer::Base.deliveries. This improves test isolation for parallel
execution.

Changes:
- 'creates an invitation for each student' now verifies emails sent via delivery count
- 'creates an invitation for each coach' now verifies emails sent via delivery count
- Tests verify recipient email addresses match expected members
- Removes expect(mailer).to receive(:invite_student/coach) mocks
…ertions

Replaces mock expectations with ActionMailer::Base.deliveries count
assertions for #send_waiting_list_emails tests.

Changes:
- 'emails coaches when there are free coach spots' - verifies email delivery
- 'does not email coaches when no coach spots' - verifies no emails sent
- 'emails students when there are free student spots' - verifies email delivery
- 'does not email students when no student spots' - verifies no emails sent

Removes expect(WorkshopInvitationMailer).to receive(:notify_waiting_list) mocks
Replaces mock expectations with ActionMailer::Base.deliveries count
assertions for reminder email tests.

Changes:
- 'emails all attending members' (monthly) - uses _without_delay method
- 'emails all attending members' (workshop) - verifies email delivery count
- 'emails waiting list' - uses _without_delay method

Notes:
- Monthly and waiting list tests remain marked :wip as they were before
- These methods are async and require _without_delay for sync testing

Removes expect(Mailer).to receive(:attendance_reminder) mocks
Replaces mock expectations with ActionMailer::Base.deliveries count
assertions for #send_meeting_emails tests.

Changes:
- 'emails all invitees that are not banned' - verifies email count excludes banned
- 'emails valid invitees only once' - verifies email count excludes already invited

Uses _without_delay method for synchronous testing.

Removes expect(MeetingInvitationMailer).to receive(:invite) mocks
Replaces mock expectations with ActionMailer::Base.deliveries count
assertions for async behavior tests.

Changes:
- 'sends invitation emails' (async context) - verifies email delivery
- 'sends attendance reminder emails' (async context) - verifies email delivery

Uses _without_delay methods for synchronous testing.

Removes remaining expect(WorkshopInvitationMailer).to receive mocks
…ertions

Simplifies after create hook tests by removing mock-based tests
and replacing with direct email delivery verification.

Changes:
- Consolidates two mock-based tests into one email delivery test
- Verifies email is sent and has expected subject

Removes allow().to receive() and have_received() mocks
…ssertions

Replaces spy-based mock test with email delivery verification.

Changes:
- 'sends an eligibility check email' - verifies email delivery count and recipient

Removes allow().to receive() and have_received() mocks
…sertions

Replaces spy-based mock test with email delivery verification.

Changes:
- 'sends an attendance warning email' - finds email by recipient and subject
- Uses flexible matching to account for other emails in deliveries

Removes allow().to receive() and have_received() mocks
… assertions

Replaces expect_any_instance_of mocks with ActionMailer::Base.deliveries
verification for all welcome email scenarios.

Changes:
- First-time coach subscription - verifies email sent and contains 'coach'
- First-time student subscription - verifies email sent and contains 'student'
- Second subscription tests - clears deliveries and verifies no new emails
- Unsubscribe/resubscribe tests - verifies no duplicate emails sent

Adds before hook to clear deliveries for consistent test isolation.

Removes all expect_any_instance_of(MemberMailer) mocks which were
problematic for parallel test execution.
…rtions

Replaces expect_any_instance_of mocks with direct mail content
verification for welcome email tests.

Changes:
- 'sends the coach welcome email to coaches' - verifies email body contains coach text
- 'sends the student welcome email to students' - verifies email body contains student text
- 'sends a ban email to a member' - verifies recipient and email body

Removes all expect_any_instance_of(MemberMailer) mocks which were
problematic for parallel test execution.
The delivery-assertion refactor incorrectly asserted that already-reminded

invitations should have reminded_at as nil. The fabricator sets

reminded_at to 2.days.ago, so the correct assertion is that it

remains unchanged (still ~2 days ago).
@mroderick mroderick force-pushed the refactor/mailer-tests branch from 58d14c1 to 662ab6a Compare May 26, 2026 19:50
@mroderick
Copy link
Copy Markdown
Collaborator Author

Note on the fix for send_monthly_attendance_reminder_emails

The test refactor in this PR converts send_monthly_attendance_reminder_emails from a mock-based assertion to a delivery assertion:

expect {
  manager.send_monthly_attendance_reminder_emails(meeting)
}.to change { ActionMailer::Base.deliveries.count }.by(attendees.count)

This would have failed before because the production method was broken — it instantiated a mailer message but never called .deliver_now or .deliver_later, so no emails were ever sent. The original :wip test and mock-based assertion masked this bug.

The production fix is in PR #2622, which adds .deliver_now to the mailer call:

# Before (broken — message discarded)
MeetingInvitationMailer.attendance_reminder(monthly, member)

# After (fixed — message delivered)
MeetingInvitationMailer.attendance_reminder(monthly, member).deliver_now

With #2622 merged, all tests in this PR now pass.

@mroderick
Copy link
Copy Markdown
Collaborator Author

@olleolleolle, this is now finally ready for your review

Copy link
Copy Markdown
Collaborator

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Perhaps: mail.body.encoded - can perhaps try expect(mail.body).to include("…”) for those checks. No big deal.)

Huzzah!

@olleolleolle olleolleolle merged commit 3810958 into codebar:master May 27, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants